Open files with spaces in filename #1231
Conversation
helix-term/src/commands.rs
Outdated
| if args.is_empty() { | ||
| bail!("wrong argument count") | ||
| } |
There was a problem hiding this comment.
| if args.is_empty() { | |
| bail!("wrong argument count") | |
| } | |
| ensure!(!args.is_empty(), "wrong argument count"); |
ath3
force-pushed
the
open-space-filename
branch
from
a336a9c
to
189eb63
Compare
helix-term/src/commands.rs
Outdated
| @@ -1923,7 +1923,8 @@ mod cmd { | |||
| args: &[&str], | |||
| _event: PromptEvent, | |||
| ) -> anyhow::Result<()> { | |||
| let path = args.get(0).context("wrong argument count")?; | |||
| ensure!(!args.is_empty(), "wrong argument count"); | |||
| let path = args.join(" "); | |||
There was a problem hiding this comment.
Shouldn't we teach the arg parser to deal with quotes instead? I can imagine using :open to open multiple files
There was a problem hiding this comment.
I reworked it to support doublequotes for the filenames with space(s), and opening multiple files at once.
ath3
force-pushed
the
open-space-filename
branch
3 times, most recently
from
8670aac
to
962e682
Compare
helix-term/src/commands.rs
Outdated
| .split_whitespace() | ||
| .collect::<Vec<&str>>(), | ||
| ); | ||
| for i in 0..quote_pos.len() / 2 { |
There was a problem hiding this comment.
Seems to me that you're trying to simulate .chunks(2) (and I can't wait for array_chunks(2) to be stabilized).
There was a problem hiding this comment.
It might also be easier to scan over the string and use a state machine, that way we can also handle escaping and also handle single quotes: https://github.com/tmiasko/shell-words/blob/8f264f1f9ec463a523c751f7bb56a07371db2b53/src/lib.rs#L108-L211
I'd use a Cow<'_, str> so that we can avoid allocating parts without escapes.
There was a problem hiding this comment.
It's OK to do this in a follow-up though.
There was a problem hiding this comment.
Thanks for the info on chunks, i rewrote that part and i think the code is now a lot cleaner and easier to understand.
There was a problem hiding this comment.
I did it like this, because this way i can just use the str slices, no need to do extra allocations.
But in case it will be needed i can try to rewrite this using state machine.
There was a problem hiding this comment.
There's no need to allocate if you use a state machine and only track offsets:
" => start = pos
.... pos++
" => start.is_some() -> end = pos
words.push(&input[start..end])
We only need to allocate if there's an escape character present. That's why I was suggesting Cow<'_, str>
There was a problem hiding this comment.
Ah yes, i understand it now, thanks for the explanation!
There was a problem hiding this comment.
I made the changes, and it now supports quotes, doublequotes, escaping, also open and hsplit/vsplit can now open multiple files at once.
ath3
force-pushed
the
open-space-filename
branch
2 times, most recently
from
3dc5520
to
ee6f014
Compare
helix-term/src/commands.rs
Outdated
| cx.editor.set_error(format!("{}", e)); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Handle typable commands | ||
| if let Some(cmd) = cmd::TYPABLE_COMMAND_MAP.get(parts[0]) { | ||
| if let Err(e) = (cmd.fun)(cx, &parts[1..], event) { | ||
| enum State { |
There was a problem hiding this comment.
Can you move the parser into a new module in helix-core (shellwords maybe?) and add some unit tests?
There was a problem hiding this comment.
Yes, i pushed the changes.
ath3
force-pushed
the
open-space-filename
branch
from
ee6f014
to
09c3e10
Compare
ath3
force-pushed
the
open-space-filename
branch
from
09c3e10
to
0de1c8c
Compare
No description provided.